-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ignore IIFE's in the no-loop-func
rule
#17528
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Can you also add a note in the rule details that explains that IIFEs are ignored while generators/async are not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a suggestion clean up the docs.
I think we could simplify this a lot by taking a different approach, that is to skip IIFEs (that are not async, generators, or self-referencing) here and thus let the rule check and report their inner functions directly. eslint/lib/rules/no-loop-func.js Lines 47 to 52 in bd7a71f
For example, the current version of this rule reports error for unsafe reference to /* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
arr.push((
() => { // <-- error here
return () => i;
}
)());
} After this change, it would report the same error directly on the inner function: /* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
arr.push((
() => {
return () => i; // <-- error here
}
)());
}
The only downside is that this can change error locations (and thus existing |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Not Stale. @snitin315 is less active at the moment. Will be back soon. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
@mdjermanovic Once we changed error locations for |
I think it's okay in this case. For |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Makes sense, I'll update the PR 👍🏻 |
18cddef
to
06e94c4
Compare
This example should now be moved into the examples of correct code: eslint/docs/src/rules/no-loop-func.md Lines 44 to 46 in 389744b
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few suggestions about the tests, then LGTM.
tests/lib/rules/no-loop-func.js
Outdated
for (var i = 0; i < 5; i++) { | ||
arr.push((f => f)(() => i)); | ||
} | ||
`, | ||
languageOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (var i = 0; i < 5; i++) { | |
arr.push((f => f)(() => i)); | |
} | |
`, | |
languageOptions: { ecmaVersion: 6 }, | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
for (var i = 0; i < 5; i++) { | |
arr.push((f => f)( | |
() => i | |
)); | |
} | |
`, | |
languageOptions: { ecmaVersion: 6 }, | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 6 }] |
In tests with nested functions of same type, it would be good to add locations to clarify which of the functions is reported.
tests/lib/rules/no-loop-func.js
Outdated
} | ||
`, | ||
languageOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 6 }] |
tests/lib/rules/no-loop-func.js
Outdated
} | ||
`, | ||
languageOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 6 }] |
tests/lib/rules/no-loop-func.js
Outdated
return () => (() => i)(); | ||
})()); | ||
} | ||
`, | ||
languageOptions: { ecmaVersion: 6 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return () => (() => i)(); | |
})()); | |
} | |
`, | |
languageOptions: { ecmaVersion: 6 }, | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
return () => | |
(() => i)(); | |
})()); | |
} | |
`, | |
languageOptions: { ecmaVersion: 6 }, | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 6 }] |
tests/lib/rules/no-loop-func.js
Outdated
} | ||
`, | ||
languageOptions: { ecmaVersion: 2022 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 6 }] |
tests/lib/rules/no-loop-func.js
Outdated
} | ||
`, | ||
languageOptions: { ecmaVersion: 2022 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'j'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors: [{ messageId: "unsafeRefs", data: { varNames: "'j'" }, type: "ArrowFunctionExpression" }] | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'j'" }, type: "ArrowFunctionExpression", line: 7 }] |
tests/lib/rules/no-loop-func.js
Outdated
} | ||
`, | ||
languageOptions: { ecmaVersion: 2022 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 7 }] |
tests/lib/rules/no-loop-func.js
Outdated
} | ||
`, | ||
languageOptions: { ecmaVersion: 2022 }, | ||
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }] | |
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression", line: 3 }] |
Co-authored-by: Milos Djermanovic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Since this is marked as a feature, it needs another approval from a team member before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Fixes #16902
Is there anything you'd like reviewers to focus on?